Skip to content

Add configurable host binding for external access#13

Closed
crankycoder wants to merge 12 commits into
selimacerbas:mainfrom
crankycoder:main
Closed

Add configurable host binding for external access#13
crankycoder wants to merge 12 commits into
selimacerbas:mainfrom
crankycoder:main

Conversation

@crankycoder
Copy link
Copy Markdown

@crankycoder crankycoder commented Apr 10, 2026

  • New host config option — bind to 0.0.0.0 for external access (default: 127.0.0.1)
  • Fix stale lock file silently preventing browser launch — now verifies the lock owner PID is alive before trusting the port
  • Add CI workflow (stable + nightly Neovim) and lock semantics test suite

Copy link
Copy Markdown
Owner

@selimacerbas selimacerbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @crankycoder, thanks for this — the host config itself is clean and well-tested, and we just shipped the live-server.nvim side in v1.3.0.

Before merging this PR, could you split it into two? The reason: it bundles two independent feature sets, and they should be reviewable / revertable separately.

PR A: host config (this PR, scoped down)

  • host config field
  • lock.is_server_alive(host, port) signature change
  • remote.send_event(host, port, ...) signature change
  • Browser URL uses configured host
  • The test harness (tests/host_config_test.lua)

PR B: browser opening improvements (new PR)

The changes in util.open_in_browser() are useful but unrelated to host config:

  1. vim.notify URL always
  2. vim.ui.open fallback
  3. vim.fn.executable() checks
  4. on_exit error handling

These are 4 separate improvements and they touch the same function that PR #14 (just merged) modified for WSL support — so you'll need to rebase regardless.

I'll happily merge both PRs. Splitting just keeps the history clean and makes it easier to land each piece without holding the other hostage.

Let me know if you'd prefer me to do the split for you (cherry-picking your commits into two branches) — happy to take that on if it's easier.

@crankycoder crankycoder requested a review from selimacerbas May 9, 2026 20:58
@crankycoder
Copy link
Copy Markdown
Author

sorry for the long delay. i think this should be ok now - i've rebased and thinned out the PR.

@crankycoder crankycoder marked this pull request as draft May 9, 2026 22:04
@crankycoder
Copy link
Copy Markdown
Author

hrm. couple minor issues i found that i'm working through rn

- lock.is_server_alive now verifies the lock owner PID is still alive
  before trusting the port check. Prevents false secondary detection
  when a different process reuses the port after the original owner dies.
- Add lock_test.lua with 10 tests covering PID liveness, write/read/remove
  roundtrip, corrupt files, port+PID semantics, and the stale-lock scenario.
- Add GitHub Actions workflow that auto-discovers tests/*_test.lua.
- Add CI badge and document host/port network options in README.
@crankycoder crankycoder marked this pull request as ready for review May 9, 2026 22:13
@crankycoder
Copy link
Copy Markdown
Author

alright. fixed a stale lock bug and added a couple tests around that.

@crankycoder
Copy link
Copy Markdown
Author

@selimacerbas ?

@selimacerbas
Copy link
Copy Markdown
Owner

Thanks @crankycoder, sorry for the delay on my side. The split looks great, the stale-lock fix is a good catch, and the tests are exactly what we needed (especially the port-reuse scenario in section 4 of lock_test.lua). Two small things before merge, plus context on a follow-up.

Before merge

1. README security warning. When a user sets host = "0.0.0.0", they expose more than the rendered preview. Anyone on the same network can:

  • read content.md (the markdown buffer being edited)
  • hit /__live/inject?event=reload&data=... to trigger reloads or inject scroll events into the user's browser tab (the endpoint has no auth)

Could you add a > [!WARNING] block in the Network section saying something like:

> [!WARNING]
> Binding to `0.0.0.0` exposes the preview server (and the current
> markdown buffer) to anyone on the same network. The live-reload
> control endpoints are currently unauthenticated. Only use this on
> trusted networks. Prefer a one-shot SSH tunnel otherwise.

2. Windows verification. The PID-aliveness check uses uv.kill(pid, 0). CI only runs on ubuntu-latest. libuv's uv_kill should handle signal 0 on Windows (it maps to OpenProcess + WaitForSingleObject), but I'd like to confirm before merging. Either add windows-latest to the matrix, or share a quick screen-grab from Windows nvim showing :lua print(vim.loop.kill(vim.fn.getpid(), 0)) returning 0 for alive.

Follow-up I'll handle separately

I'm going to add token auth in live-server.nvim, Jupyter style. Random 128-bit token at server start, embedded in index.html, gated on all sensitive endpoints, written to the lockfile (mode 0600) so secondary instances can still hit /__live/inject. That makes host = "0.0.0.0" actually safe to recommend. Bigger change, separate PR in the sibling repo, no churn for your PR here. When it lands I'll soften the README warning.

Once the warning is in and Windows is confirmed, this is good to go. Thanks again for the patience and the tests.

The fallback connected to 127.0.0.1:8421 to check PID liveness, which
could falsely report a dead PID as alive if anything was listening on
that port. Signal 0 is the authoritative check — if the kernel says the
PID doesn't exist, it doesn't exist.
Combines dynamic host binding with upstream's token auth query param.
@selimacerbas
Copy link
Copy Markdown
Owner

Thanks @crankycoder, the README warning is in (the inline **⚠️ Warning:** formatting works fine, no need to change to > [!WARNING]). One real problem with the lock.lua change though, and a heads-up on conflicts.

lock.lua is_pid_alive regression

I tested locally (vim.loop.kill(99999999, 0) returns nil ESRCH, kill(self_pid, 0) returns 0) and the new version reintroduces the exact bug your own section 4 test was designed to catch. Walking through:

function M.is_pid_alive(pid)
    local ok = uv.kill(pid, 0)
    if ok ~= nil then return true end
    -- TCP fallback to 8421
    local alive = false
    tcp:connect("127.0.0.1", 8421, function(err) alive = not err; ... end)
    vim.wait(100, function() return alive ~= nil end, 10)
    ...
    return alive
end

Three issues:

  1. Dead PID falls into the wrong question. On Unix, dead PID makes uv.kill return nil, then we fall into the TCP fallback that asks "is anything on port 8421?" instead of "is this PID alive?" Different question. The whole point of your original PID check was to catch stale locks where the port has been reused by a different process. The fallback defeats that.

  2. The fallback always returns false due to a variable-init bug. local alive = false (not nil). Then vim.wait(100, function() return alive ~= nil end, 10). Since false ~= nil is true, vim.wait returns in microseconds without pumping the TCP callback. The callback fires asynchronously after we've already returned, so the variable update is meaningless. I confirmed with vim.loop.hrtime() measurement, elapsed time is 0.001ms. Net effect: the fallback always returns false, and there's also a TCP socket leak in the non-nil branch (never closed).

  3. Port 8421 is hardcoded. Wrong in multi mode where ports are OS-assigned, and is_pid_alive is supposed to be port-agnostic anyway.

  4. Windows reality. I checked libuv source: uv_kill(pid, 0) has worked on Windows since v1.41 (2021), refined in v1.44 and v1.49. It maps to GetExitCodeProcess + WaitForSingleObject and returns 0 for live, UV_ESRCH for dead. Neovim 0.10+ bundles libuv 1.46+, so vim.loop.kill(pid, 0) behaves the same on Windows as on Unix. If you actually saw nil for a known-live PID on Windows, share your nvim version and :lua print(vim.loop.version_string()) and we'll look at it properly.

Recommendation: revert is_pid_alive to the original two-line form:

function M.is_pid_alive(pid)
    local ok = uv.kill(pid, 0)
    return ok ~= nil
end

libuv handles both platforms; no fallback needed.

Conflicts with main

Since this PR was opened, four PRs landed that touched the same files: #16 (default_theme), #22 (ELK), #23 (token auth in init.lua/lock.lua/remote.lua), #24 (lifecycle hooks). Concrete clashes:

  • lock.lua: main has M.write(port, workspace, token) and mode 0600. This branch has the old 2-arg version.
  • remote.lua: main has send_event(port, type, json, token). This branch has send_event(host, port, type, json) — different signature, different arg order.
  • init.lua: main passes token and protected_paths to ls_server.start, generates a token at start, and uses a browser_url(port) helper everywhere.

The host config piece is still valuable and we still want to merge it. But the rebase isn't trivial. If you'd rather not deal with it, say the word and I'll rebase + fix is_pid_alive myself on a fresh branch, with you as Author on the resulting commits.

Integrates upstream's token auth, hooks, browser_url helper, ELK layout,
default_theme, and protected_paths — while layering in dynamic host binding
for the server, lock check, scroll sync, and browser URL.
Adopts upstream's token-in-lockfile feature (needed for secondary instance
auth) and the restrictive file permission fix, while keeping our PID-based
stale lock detection.
Resolves all conflicts:
- init.lua: keeps upstream token auth + hooks + browser_url, layers in host
- lock.lua: keeps upstream token in lockfile + 0600 perms, layers in PID check + host
- remote.lua: keeps upstream token query param, layers in dynamic host
- README.md: keeps both Network and Hooks sections
@crankycoder
Copy link
Copy Markdown
Author

i'm gonna close this PR and just recreate it from first principles - the merge conflicts are pretty painful rn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants